Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: track and query protocol rev across all modules #6804

Merged
merged 21 commits into from
Nov 10, 2023

Conversation

czarcas7ic
Copy link
Member

@czarcas7ic czarcas7ic commented Nov 1, 2023

Closes: #XXX

What is the purpose of the change

The Osmosis protocol now generates multiple streams of revenue:

  1. Taker fees via the poolmanager module
  2. Tx fees via txfees module
  3. Cyclic arbs via the protocol rev module

We currently only explicitly track the cyclic arb profits. Calculating tx fees and taker fees is complicated, and tracking via the protocol itself at time of charge is much simpler.

This PR adds trackers to taker fees, tx fees, and a new cyclic arb tracker (the previous one didn't track the start height in which the profits began from), and implements a query in the protorev module that fetches and serves these values in a single query. The trackers themselves live in each respective module (taker fee tracker KVStore in poolmanager, tx fees KVStore in txfees, and cyclic arbs KVStore in protorev), with the caller that amalgamates these result living in the protorev module itself.

Testing and Verifying

Go tests added for all newly introduced logic

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

  • Specification (x/{module}/README.md)
  • Osmosis documentation site
  • Code comments?
  • N/A

@czarcas7ic czarcas7ic added the V:state/breaking State machine breaking PR label Nov 1, 2023
Copy link
Contributor

github-actions bot commented Nov 1, 2023

Important Notice

This PR modifies an in-repo Go module. It is one of:

  • osmomath
  • osmoutils
  • x/ibc-hooks
  • x/epochs

The dependent Go modules, especially the root one, will have to be
updated to reflect the changes. Failing to do so might cause e2e to fail.

Please follow the instructions below:

  1. Open https://github.com/osmosis-labs/osmosis/actions/workflows/go-mod-auto-bump.yml
  2. Provide the current branch name
  3. On success, confirm if an automated commit corretly updated the go.mod and go.sum files

Please let us know if you need any help.

@czarcas7ic czarcas7ic changed the title feat: query protocol rev across all modules feat: track and query protocol rev across all modules Nov 1, 2023
github-actions and others added 2 commits November 1, 2023 19:13
@@ -0,0 +1,85 @@
package apptesting
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The methods in the file are extracted from previously existing txfees helpers because they need to be used in other module tests, and I wanted to prevent code duplication.

@@ -114,3 +114,11 @@ func MergeCoinMaps[T comparable](currentEpochExpectedDistributionsOne map[T]sdk.
}
return newMap
}

func ConvertCoinArrayToCoins(coinArray []sdk.Coin) sdk.Coins {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While coin arrays can be used in place of a coins object and still be valid, coin array do not posses the same sub methods (ex. Sub, Add, etc), so this method is useful imo.

@@ -19,7 +19,7 @@ func (s *KeeperTestSuite) TestChargeTakerFee() {

var (
defaultTakerFee = osmomath.MustNewDecFromStr("0.01")
defaultAmount = sdk.NewInt(100)
defaultAmount = sdk.NewInt(10000000)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was increased because with a taker fee as small as 0.01 and an input of 100, the split between taker fee and community pool is too small to actually test. In other words, the taker fee is 1, and splitting this between the two does not work (in practice/mainnet what happens is, the first category of staking rounds it down to 0, and then the remainder (1) is all sent to the second category)

@czarcas7ic czarcas7ic marked this pull request as ready for review November 2, 2023 04:30
Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, left some minor nit comments, otherwise looks good to me! Nice work on implementing all this across the modules !

x/poolmanager/protorev.go Show resolved Hide resolved
proto/osmosis/poolmanager/v1beta1/genesis.proto Outdated Show resolved Hide resolved
x/txfees/keeper/genesis.go Show resolved Hide resolved
x/txfees/keeper/hooks_test.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@PaddyMc PaddyMc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work 💪

@czarcas7ic czarcas7ic closed this Nov 10, 2023
@czarcas7ic czarcas7ic reopened this Nov 10, 2023
@czarcas7ic czarcas7ic closed this Nov 10, 2023
@czarcas7ic czarcas7ic reopened this Nov 10, 2023
@czarcas7ic czarcas7ic added V:state/breaking State machine breaking PR and removed V:state/breaking State machine breaking PR labels Nov 10, 2023
@czarcas7ic czarcas7ic merged commit 56d665b into main Nov 10, 2023
1 check passed
@czarcas7ic czarcas7ic deleted the adam/all-protocol-rev-query branch November 10, 2023 06:37
@github-actions github-actions bot mentioned this pull request Apr 1, 2024
@github-actions github-actions bot mentioned this pull request Apr 15, 2024
@github-actions github-actions bot mentioned this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:app-wiring Changes to the app folder C:CLI C:x/poolmanager C:x/txfees V:state/breaking State machine breaking PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants